-
Notifications
You must be signed in to change notification settings - Fork 1.9k
refactor: replace go-multierror with errors.Join for native error aggregation #8791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: replace go-multierror with errors.Join for native error aggregation #8791
Conversation
|
/kind cleanup |
|
The following is the coverage report on the affected files.
|
5aa95e5 to
b8b0c58
Compare
|
The following is the coverage report on the affected files.
|
b8b0c58 to
95bc916
Compare
95bc916 to
f8b8169
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
refactor season 🌴 |
7cd4853 to
66f015b
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
aThorp96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change! It's a shame the dependency is still indirect but removing direct dependence on tiny dependencies which don't add that much utility is nonetheless really valuable!
|
|
||
| var merr *multierror.Error | ||
| var errs []error | ||
| if err := setTaskRunStatusBasedOnStepStatus(ctx, logger, stepStatuses, &tr, pod.Status.Phase, kubeclient, ts); err != nil { | ||
| merr = multierror.Append(merr, err) | ||
| errs = append(errs, err) | ||
| } | ||
|
|
||
| setTaskRunStatusBasedOnSidecarStatus(sidecarStatuses, trs) | ||
|
|
||
| trs.Results = removeDuplicateResults(trs.Results) | ||
|
|
||
| return *trs, merr.ErrorOrNil() | ||
| return *trs, errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like `errors.join is necessary here, since we're only ever have one error. From what I can tell this is all that's necessary in this case:
err := setTaskRunStatusBasedOnStepStatus(ctx, logger, stepStatuses, &tr, pod.Status.Phase, kubeclient, ts)
setTaskRunStatusBasedOnSidecarStatus(sidecarStatuses, trs)
trs.Results = removeDuplicateResults(trs.Results)
return *trs, err| func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1.TaskRun, podPhase corev1.PodPhase, kubeclient kubernetes.Interface, ts *v1.TaskSpec) error { | ||
| trs := &tr.Status | ||
| var merr *multierror.Error | ||
| var errs []error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're joining them all at the end anyway, any reason we're not going something like this?
var errs error
slr, err := sidecarlogresults.GetResultsFromSidecarLogs(ctx, kubeclient, tr.Namespace, tr.Status.PodName, pipeline.ReservedResultsSidecarContainerName, podPhase)
errs = errors.Join(errs, err) // note that errors.Join handles err being nil internally :)
...
err = setTaskRunArtifactsFromRunResult(sidecarLogResults, &tras)
errs = errors.Join(errs, err)
return errslist appending isn't free, so it seems like using errors.Join each time may be preferable, but maybe not. Not sure what the best practice is here, hence my asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Looking at the implementation, errors.Join might not be the preferred way, as it loops through the errors multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. Amortized errors.Join is going to result in more allocations in addition to the extra iteration, since append isn't going to make a new array every call but make certainly definitely will...
pkg/pod/status_test.go
Outdated
| }) | ||
| var wantErr *multierror.Error | ||
| wantErr = multierror.Append(wantErr, c.wantErr) | ||
| wantErr := errors.Join(c.wantErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary anymore, since we're no longer dealing the with MultiError custom type, and just dealing with the error type. We should be able to get away with just using c.wantErr without a wrapper. See this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm mistaken, you want me to do something like this?:
gotErr := setTaskRunStatusBasedOnStepStatus(ctx, logger, []corev1.ContainerStatus{{}}, &c.tr, pod.Status.Phase, kubeclient, ts)
if gotErr == nil {
t.Fatalf("Expected error but got nil")
}
if d := cmp.Diff(c.wantErr.Error(), gotErr.Error()); d != "" {
t.Errorf("Got unexpected error %s", diff.PrintWantGot(d))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly :)
pkg/pod/status_test.go
Outdated
| if merr == nil { | ||
| t.Fatalf("Expected error but got nil") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is no longer a multierror, do you mind renaming the variable to something more accurate? Maybe gotErr or err?
pkg/reconciler/taskrun/taskrun.go
Outdated
| var errs []error | ||
| if previousError != nil { | ||
| errs = append(errs, previousError) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether or not we opt to use a errors.Join instead of append(errs, err), we can take advantage of errors.Join's ability to ignore nil input:
(if using errors.Join for each error - if previousErr is nil errors.Join() will ignore it at the next call to errors.Join(errs, err))
| var errs []error | |
| if previousError != nil { | |
| errs = append(errs, previousError) | |
| } | |
| errs := previousError |
(if using append(errs, err) for each error, errors.Join will ignore a nil entry in the list when we return)
| var errs []error | |
| if previousError != nil { | |
| errs = append(errs, previousError) | |
| } | |
| errs := []error{previousError} |
|
The following is the coverage report on the affected files.
|
a262cc1 to
3419aa9
Compare
|
The following is the coverage report on the affected files.
|
3419aa9 to
3be56c3
Compare
|
The following is the coverage report on the affected files.
|
aThorp96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
@aThorp96: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aThorp96, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Changes
This PR removes the dependency on the external
go-multierrorpackage and replaces it with Go's built-inerrors.Joinfunctionality for aggregating multiple errors.Fixes #8776
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes